-
Notifications
You must be signed in to change notification settings - Fork 58
[WIP] Annotated completions #711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's proceeding well.
Unsure about the naming of some variants, e.g. ResolvedType
. Perhaps just adding some comments to the definition would clarify.
(Completable.Cexpression | ||
{contextPath = ctxPath; prefix = ""; nested = []}) | ||
| _ -> ()) | ||
| {pvb_pat; pvb_expr} when locHasCursor pvb_pat.ppat_loc -> ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it clear that this case cannot be an instance of the cases above? Are we relying on the order of cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly sure we're not depending on the ordering, but we're getting dangerously close...
@@ -296,6 +294,36 @@ end | |||
|
|||
type polyVariantConstructor = {name: string; args: Types.type_expr list} | |||
|
|||
(** An type that can be used to drive completion *) | |||
type completionType = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about factoring out env
? Seems to be present in every case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea. I'll need to have a deep look at how env is passed around, where it's needed, etc. Ok with you if I do that in a separate PR?
@@ -582,8 +584,22 @@ let completionsGetTypeEnv = function | |||
| {Completion.kind = Field ({typ}, _); env} :: _ -> Some (typ, env) | |||
| _ -> None | |||
|
|||
let completionsGetCompletionType ~full = function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every case seems to return the same env
. Factor out?
| Tuple of QueryEnv.t * Types.type_expr list * Types.type_expr | ||
| Toption of QueryEnv.t * completionType | ||
| Tbool of QueryEnv.t | ||
| Tarray of QueryEnv.t * completionType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at env
in the nested case, e.g. in the argument to Tarray
.
It seems to me, from a cursory look, that the inner env might always be the same as the outer env.
If true in general, then factoring out the env
in the type definition means it does not need to appear in the argument of Tarray
etc.
I haven't really checked that this is true for all cases. If it is true, then the overall type would be:
{env; desc}
where desc
would be the recursive definition of the variant cases. So e.g. Tarray of desc
would not include the env
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great.
Left some comments about possible further refactoring opportunities.
Completion via annotations. E.g:
let x: someVariant = O<com>
completes for the variantsomeVariant
, even if the let assignment hasn't compiled previously.Only handles "basic" types as of now. No handling of options etc.
Currently quite messy, but looking for your feedback before I continue @cristianoc .